Skip to content

Remove the file_manager module#130

Merged
rwb27 merged 4 commits intomainfrom
retire-file-manager
Jul 3, 2025
Merged

Remove the file_manager module#130
rwb27 merged 4 commits intomainfrom
retire-file-manager

Conversation

@rwb27
Copy link
Copy Markdown
Collaborator

@rwb27 rwb27 commented Jul 2, 2025

file_manager was a previous attempt to handle actions that created files. It is retired in favour of the Blob class. I'm not aware of any examples of file_manager being used "in the wild" and it
does not do anything that Blob can't do.

We are therefore getting rid of it as vestigial code.

@barecheck
Copy link
Copy Markdown

barecheck bot commented Jul 2, 2025

Barecheck - Code coverage report

Total: 89.18%

Your code coverage diff: 0.63% ▴

Uncovered files and lines
FileLines
src/labthings_fastapi/actions/__init__.py140, 192-198, 281-282, 358-359, 364, 372, 391-392
src/labthings_fastapi/descriptors/action.py145-146, 150, 156, 158, 229-230, 233, 257

@rwb27 rwb27 force-pushed the retire-file-manager branch from a22beca to ba398e9 Compare July 2, 2025 21:51
rwb27 added 3 commits July 2, 2025 22:59
`file_manager` was a previous attempt to handle actions that created files. It is retired in favour of the `Blob` class. I'm not aware of any examples
of `file_manager` being used "in the wild" and it
does not do anything that `Blob` can't do.

We are therefore getting rid of it as vestigial code.
This is a bonus extra test, to stop the coverage diff being negative after deleting FileManager.
@rwb27 rwb27 force-pushed the retire-file-manager branch from ba398e9 to b21c7f1 Compare July 2, 2025 22:01
@rwb27 rwb27 requested a review from julianstirling July 2, 2025 22:12
@rwb27
Copy link
Copy Markdown
Collaborator Author

rwb27 commented Jul 2, 2025

The coverage diff was negative, so I've made some testing improvements:

  1. I've tested the endpoint that lists all invocations in test_action_manager.py
  2. I've excluded all the TYPE_CHECKING import blocks with an entry in .coveragerc

Neither is required to remove the file manager, but both are useful and I thought I'd be better off spending the time adding a test than justifying why I want to merge when the coverage diff is negative.

Copy link
Copy Markdown
Contributor

@julianstirling julianstirling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this

@rwb27 rwb27 merged commit 7957450 into main Jul 3, 2025
14 checks passed
@rwb27 rwb27 deleted the retire-file-manager branch July 3, 2025 00:25
@rwb27 rwb27 mentioned this pull request Mar 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants